-
Notifications
You must be signed in to change notification settings - Fork 50
Conversation
Implemented webRTC secure signalling application on SAFE Network.
(maidsafe_highfive has picked a reviewer for you, use r? to override) |
Removed credentials
Update readme.md for configuration of STUN and TURN server
Resolve public name issues
safe_webrtc_example/app/constants.js
Outdated
CONFIG: { | ||
SERVER: { | ||
iceServers: [ | ||
{ url: 'stun:stun1.l.google.com:19302' }, // URL to STUN Server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps replace the value with a placeholder similar to how it's done with the credential and username.
safe_webrtc_example/app/constants.js
Outdated
iceServers: [ | ||
{ url: 'stun:stun1.l.google.com:19302' }, // URL to STUN Server | ||
{ | ||
url: 'turn:numb.viagenie.ca', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
safe_webrtc_example/app/constants.js
Outdated
ERR_CODE: { | ||
NO_SUCH_ENTRY: -106, | ||
}, | ||
CRYPTO_KEYS: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this can be removed.
safe_webrtc_example/README.md
Outdated
iceServers: [ | ||
{ url: 'stun:stun1.l.google.com:19302' }, // URL to STUN Server | ||
{ | ||
url: 'turn:numb.viagenie.ca', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put placeholders here rather than google stun server url's.
safe_webrtc_example/app/safe_comm.js
Outdated
resolve(true); | ||
} catch (err) { | ||
// if (err.code !== CONST.ERR_CODE.NO_SUCH_ENTRY) { | ||
if (err.message !== ERROR_MSG.ENTRY_NOT_FOUND) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming error codes are not available from the DOM API? If so, I would add a comment (TODO/FIXME) here to state that we should use error codes whenever they become available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments for things I'd change, but I thing the most important thing would be that this PR is breaking the CI, apparently just due to a missing test
script?
Improved UI/UX features of the application. Removed server details. Localised texts.
disable logs for production
Improved label and message texts to make things consistent throughout the application.
Replace place holder instead of credentials.
Fixed travis issue, failing to find test script on root package.json
Implemented webRTC secure signalling application on SAFE Network.